Skip to content

Disallow enums in ArrayObject #15775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

iluuu1994
Copy link
Member

We should probably disallow objects in ArrayObject altogether. It allows overriding readonly properties which can break engine assumptions.

@cmb69
Copy link
Member

cmb69 commented Sep 6, 2024

Isn't the whole point of ArrayObject: "This class allows objects to work as arrays."? (what may not have been the best idea, though)

@iluuu1994
Copy link
Member Author

iluuu1994 commented Sep 6, 2024

I have no clue what the point is, tbh. I really don't understand when that would be useful. In any case, this allows to circumvent both type and readonly checks (https://3v4l.org/KOWT9), which can both violate assumptions. Types are obvious. Readonly can break internal enum name checks when the name of internal enums is changed.

@cmb69
Copy link
Member

cmb69 commented Sep 6, 2024

My point was more that instead of disallowing objects in ArrayObject, maybe we should deprecate that class instead. And until it is removed, document its limitations.

And frankly, not only ArrayObject but also some other parts of SPL look pretty much immature to me.

@iluuu1994
Copy link
Member Author

Indeed. Unfortunately, there's quite a bit of use of ArrayObject in the wild. For now, I'll check if it's possible to uphold property types and readonly for internal classes, where the actual assertions can occur. I don't object to deprecating ArrayObject in the future, but I also don't care if it no longer causes unsoundness.

@iluuu1994
Copy link
Member Author

@cmb69 Having looked at this more, you were right. This class is pure madness. Basically none of the operations behave like they should. Some of the issues:

  • Types are not respected (as mentioned). This goes for read+r and read+w.
  • Readonly is not respected (as mentioned). This goes for write and unset.
  • Foreach over class with readonly is broken. There's already some hacky code trying to fix it, but it doesn't work for nested ArrayObject instances, which is the case for getIterator(). There are also some reference tracking assertions being raised.
  • References are broken. Most writes ignore and thus overwrite references.
  • The code is absolute carnage. It handles extremely weird edge cases (nested ArrayObject, ArrayObject with self as the object, extended ArrayObject), which make it hard to anticipate unwanted side-effects when tweaking the logic.

IMO, we should try to understand the use-cases online, see whether they have good alternatives, and then deprecate this class.

@cmb69
Copy link
Member

cmb69 commented Sep 7, 2024

Thanks for having a closer look at the implementation!

I would presume that ArrayObject is often just used for convenience, to not having to implement the interfaces "manually". Some closer investigation might be in order, but maybe just asking on the list can shed some light on how deprecating ArrayObject would be received by the community.

@jimwins
Copy link
Member

jimwins commented Sep 7, 2024

Agreed with @cmb69 that there should be more warnings and documentation of the limitations of ArrayAccess and any other parts of the SPL that aren't fully baked, otherwise the number of projects using them is just going to grow until they are formally deprecated.

@Girgias
Copy link
Member

Girgias commented Sep 9, 2024

Note that ArrayIterator is also closely tied to the implementation of ArrayObject, but I agree that we should kill this class with fire as it violates every possible invariant in existence.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Sep 18, 2024

@cmb69 @nielsdos @bwoebi Given recent discussions on ArrayObject issues, is this reasonable first step? ArrayObject allows to modify the readonly name and value properties. This is not only highly confusing (e.g. the value will diverge from the from/tryFrom map) but also breaking for internal enums, because of code like this:

if (bounds) {
zval *case_name = zend_enum_fetch_case_name(bounds);
zend_string *bounds_name = Z_STR_P(case_name);
bounds_type = ZSTR_VAL(bounds_name)[0] + ZSTR_LEN(bounds_name);
}
switch (bounds_type) {
case 'C' + sizeof("ClosedOpen") - 1:
if (UNEXPECTED(max <= min)) {
zend_argument_value_error(2, "must be greater than argument #1 ($min)");
RETURN_THROWS();
}
RETURN_DOUBLE(php_random_gammasection_closed_open(randomizer->engine, min, max));
case 'C' + sizeof("ClosedClosed") - 1:

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes imo.
Not sure about the wording as it seems to imply that only some particular enums are incompatible. I would change it to "Enums are not compatible (...)" rather than "Enum %s is not compatible".

@Girgias
Copy link
Member

Girgias commented Sep 20, 2024

I am fine with disallowing enums, as those are rather a recent addition and unlikely to have been used in conjunction with ArrayObject.

@iluuu1994 iluuu1994 closed this in d21bc7f Sep 26, 2024
@iluuu1994
Copy link
Member Author

I decided to only merge this for master. Woe is you if you're relying on this behavior.

jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Oct 1, 2024
cmb69 added a commit to cmb69/doc-en that referenced this pull request Oct 11, 2024
See <php/php-src#15775 (comment)>.
While a formal deprecation process is pending, it seems prudent to not
advertise this class any longer.
cmb69 added a commit to php/doc-en that referenced this pull request Oct 13, 2024
See <php/php-src#15775 (comment)>.
While a formal deprecation process is pending, it seems prudent to not
advertise this class any longer.
mumumu added a commit to php/doc-ja that referenced this pull request Dec 8, 2024
See <php/php-src#15775 (comment)>.
While a formal deprecation process is pending, it seems prudent to not
advertise this class any longer.

php/doc-en@3994d01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants